refactor pipeline validation unit tests v1beta1 #2581

pritidesai · 2020-05-07T23:57:30Z

Changes

This change is part of the larger refactoring effort to simplify unit tests
by splitting one huge validate test function into its own unit tests.
The end goal is to move the individual validation functions into their own
packages or have them available as attributes of the fields they are validating
instead of defining them locally under each APIs such as v1beta1.

Most changes done in test file, split one huge test function into small tests and added more test cases.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

tekton-robot · 2020-05-08T00:01:13Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_validation.go	92.0%	91.4%	-0.6

tekton-robot · 2020-05-08T05:37:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_validation.go	92.0%	94.5%	2.5

pritidesai · 2020-05-08T06:10:15Z

@bobcatfish I have split one huge unit test into individual validations, if these changes look good, I will apply similar changes to v1alpha1 😱😓

vdemeester · 2020-05-08T11:21:34Z

@pritidesai #2537 made v1beta1 builder to ease the port. Also, this is gonna conflict hard with #2577 (as I am porting all the tests to v1beta1, starting with the builders to limit the size).
Also #2577 will make sharing code between the two a bit useless (or less important), so I think we should wait for it to get merged and #2410 to be fixed to do too much.

pritidesai · 2020-05-08T17:25:50Z

@vdemeester as much I feel I am starting to understand the whole structuring of unit tests, I dont think I do 😭

v1beta1 pipeline validation unit tests does not use tb whereas v1alpha1 does. Are we deprecating tb or adopting it back or its completely out of context here? 🤔

vdemeester · 2020-05-11T09:00:45Z

@vdemeester as much I feel I am starting to understand the whole structuring of unit tests, I dont think I do sob

v1beta1 pipeline validation unit tests does not use tb whereas v1alpha1 does. Are we deprecating tb or adopting it back or its completely out of context here? thinking

Long term goal is to deprecate them (mainly because the benefit they bring might not be worth), see #2537 (comment). But this is long-term view, and a PR like #2577 would be ever bigger (and would take way more time) if I hadn't ported the builder to v1beta1 😅

pritidesai · 2020-05-11T19:01:22Z

thanks @vdemeester, I am refactoring v1alpha1 pipeline validation and will drop tb from existing test cases in pipeline_validation_test.go at least we can start with one 😜

bobcatfish

Looks great to me!! The coverage has gone up and you improved the naming of so many existing test cases 🙏

I'm so excited to see all the focused test cases for each individual function, thanks for extracting them all out, this must have been a ton of work!!

My one request is that I'd love to see these with separate tests for success and failure cases, i.e. instead of a boolean "failureExpected" type flag, two totally different tests. I find this makes it much easier to tell what is covered, b/c you can look at the success and failure cases separately, and because you don't have to worry about the logic that is checking that flag having any bugs.

/approve

bobcatfish · 2020-05-11T21:37:38Z

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

-				Tasks: []v1beta1.PipelineTask{
-					{Name: "foo"},
-				},
+				Description: "this is a valid pipeline with all possible fields initialized",


i wonder if it makes sense to have a similar failure case pipelinespec, e.g. one with a lot of different failures (i think they'll mostly all be surfaced? not sure if the logic just bails early in that case 🤔 )

yup it does 👍

interesting!! totally outside the scope of what you're doing, but i feel like it'd be nice to be able to build up as many errors as possible and give them all at once

bobcatfish · 2020-05-11T21:39:44Z

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

+	tests := []struct {
+		name            string
+		ps              *v1beta1.PipelineSpec
+		failureExpected bool


this is definitely outside the scope of your changes b/c the tests were already here but I really recommend having completely different tests for failure/non-failure cases in general - makes them much easier to read and maintain!

e.g. if you have a test where all the pipelines are invalid, and one where they are all valid, makes it much easier to understand whats covered and whats not

in fact it looks at a glance like all the cases here might be invalid so maybe the flag isnt needed?

yup, will create separate functions for failure tests vs non-failure tests.

bobcatfish · 2020-05-11T21:43:29Z

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

+		tasks:           []v1beta1.PipelineTask{{Name: "_foo", TaskRef: &v1beta1.TaskRef{Name: "foo-task"}}},
+		failureExpected: true,
+	}, {
+		name:            "pipeline task with invalid task name (camel case)",


nice - much more descriptive than "invalid task name 2"!

bobcatfish · 2020-05-11T21:44:01Z

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

-						}},
-					}},
+		name: "invalid condition only resource -" +
+			" pipeline task condition referring to a resource which is missing from resource declarations",


thanks for adding the details 🙏

bobcatfish · 2020-05-11T21:50:36Z

pritidesai · 2020-05-11T22:31:08Z

thanks @bobcatfish, I will create separate functions for failure tests vs non-failure tests and create a separate PR with refactoring v1alpha1 pipeline validation 🥰

vdemeester

Are we exporting the validate* function just for the tests ?
If so, I really don't think this is a good idea, as this makes them part of the "tekton go API".

Maybe we want to make those function part of our "go API" and are fine with external projects depending on it, but this has to be discussed.
If we want to tests those function, we can always use the same package name instead of v1beta1_test to be able to see these. But them we are testing the impl. detail and not the go API itself ; which mean, if I refactor some validate* function without changing the main pipeline.Validate() function behaviour, I will need to update/rewrite tests, whereas I didn't have to do that before (because we were testing the contract/api not the implementation detail)

tekton-robot · 2020-05-12T18:53:49Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_validation.go	92.0%	94.5%	2.5

pritidesai · 2020-05-12T18:54:37Z

@bobcatfish @vdemeester thanks for the discussion this morning, I have updated the PR.

@bobcatfish I have split positive and negative cases into separate functions naming them _Success and _Failure, couldnt come up with better naming than this 😞 let me know if it looks ok.

This change is part of the larger refactoring effort to simplify unit tests by splitting one huge validate test function into its own unit tests. The end goal is to move the individual validation functions into their own packages or have them available as attributes of the fields they are validating instead of defining them locally under each APIs such as `v1beta1`.

tekton-robot · 2020-05-12T19:23:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_validation.go	92.0%	94.5%	2.5

vdemeester

/meow

tekton-robot · 2020-05-13T09:48:31Z

@vdemeester:

In response to this:

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2020-05-13T09:48:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bobcatfish,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bobcatfish · 2020-05-13T15:54:46Z

@bobcatfish @vdemeester thanks for the discussion this morning, I have updated the PR.

For posterity: we went back and forth a bunch on the best way to test these, in the end we all agreed (correct me if i'm wrong!) that it did make sense to have these focused tests, but that also indicated that in the long run these functions belong in a different package (we all agree that testing unexported functions isnt a good idea - it usually indicates some potential refactoring!) but in the short term we're testing the unexported functions here so that we get get the improved tests but without having to fix everything at once

thanks again @pritidesai !

/lgtm

pritidesai · 2020-05-13T16:38:35Z

thanks @bobcatfish for details, yup thats accurate 👍

tekton-robot requested review from dlorenc and vdemeester May 7, 2020 23:57

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 7, 2020

pritidesai changed the title ~~refactor pipeline validation unit tests v1beta1~~ wip: refactor pipeline validation unit tests v1beta1 May 8, 2020

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2020

pritidesai force-pushed the refactor-validation-1 branch from d6fec06 to 1e6e8a4 Compare May 8, 2020 05:33

pritidesai changed the title ~~wip: refactor pipeline validation unit tests v1beta1~~ refactor pipeline validation unit tests v1beta1 May 8, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2020

afrittoli added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 8, 2020

bobcatfish reviewed May 11, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2020

vdemeester requested changes May 12, 2020

View reviewed changes

pritidesai force-pushed the refactor-validation-1 branch from 1e6e8a4 to 09af651 Compare May 12, 2020 18:50

pritidesai force-pushed the refactor-validation-1 branch from 09af651 to 40a2a25 Compare May 12, 2020 19:20

vdemeester self-requested a review May 13, 2020 09:43

vdemeester approved these changes May 13, 2020

View reviewed changes

tekton-robot assigned bobcatfish May 13, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2020

tekton-robot merged commit 10e391c into tektoncd:master May 13, 2020

refactor pipeline validation unit tests v1beta1 #2581

refactor pipeline validation unit tests v1beta1 #2581

Uh oh!

Conversation

pritidesai commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Reviewer Notes

Uh oh!

tekton-robot commented May 8, 2020

Uh oh!

tekton-robot commented May 8, 2020

Uh oh!

pritidesai commented May 8, 2020

Uh oh!

vdemeester commented May 8, 2020

Uh oh!

pritidesai commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester commented May 11, 2020

Uh oh!

pritidesai commented May 11, 2020

Uh oh!

bobcatfish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobcatfish commented May 11, 2020

Uh oh!

pritidesai commented May 11, 2020

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 12, 2020

Uh oh!

pritidesai commented May 12, 2020

Uh oh!

tekton-robot commented May 12, 2020

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 13, 2020

Uh oh!

tekton-robot commented May 13, 2020

Uh oh!

bobcatfish commented May 13, 2020

Uh oh!

pritidesai commented May 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pritidesai commented May 7, 2020 •

edited

Loading

pritidesai commented May 8, 2020 •

edited

Loading